Fix terminal history, focus recovery, and stale PTY events#6354
Conversation
AmethystLiang
force-pushed
the
wire-terminal-worktree-navitgation-to-back-and-forth-stack
branch
from
282a0c8 to
2bc8e16
Compare
AmethystLiang
changed the title
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR updates terminal layout normalization, active leaf resolution, hydration, shutdown capture, pane binding repair, PTY exit handling, remote PTY input batching, IPC focus wiring, and visible-terminal wake recovery. It adds logic to prefer PTY-backed active leaves, repair stale selection state, clear queued input on handle changes, and recover visible terminal state on window focus or visibility changes. Tests were added and adjusted across these terminal layout and pane lifecycle paths. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/renderer/src/components/terminal-pane/pty-transport.ts (1)
509-521: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the missing why for stale PTY data gating.
The new
ptyId !== idguards encode the late-callback/rebind invariant; add one shortWhy:comment so the replay and live-data paths don’t look like redundant defensive checks.Suggested comment
// Why: relay pty.attach sends replay data via a dedicated pty:replay IPC // channel. Route it through onReplayData so the renderer engages the // replay guard and xterm auto-replies do not leak into the shell. + // Why: callbacks from an old PTY can arrive after this transport rebinds; + // ignore them instead of delivering stale output into the current pane. ptyReplayHandlers.set(id, (data) => {As per coding guidelines, “When writing or modifying code driven by a design doc or non-obvious constraint, add a comment explaining why the code behaves the way it does.”
Source: Coding guidelines
src/renderer/src/components/terminal-pane/remote-runtime-pty-transport.ts (1)
356-358: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the remote subscription identity guard.
The captured handle/PTY pair prevents late callbacks from stale streams mutating current state; add a short
Why:comment nearisCurrentSubscription.Suggested comment
const subscribedHandle = handle const subscribedPtyId = remotePtyId + // Why: stream callbacks can arrive after attach/reconnect switches handles; + // only the subscription for the current handle/PTY may mutate state. const isCurrentSubscription = (): boolean => isCurrentRemoteTerminal(subscribedHandle, subscribedPtyId)As per coding guidelines, “When writing or modifying code driven by a design doc or non-obvious constraint, add a comment explaining why the code behaves the way it does.”
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f4482e74-02c6-4193-a1f5-eee9fd83d021
📒 Files selected for processing (18)
src/main/runtime/orca-runtime.test.tssrc/main/runtime/orca-runtime.tssrc/renderer/src/components/terminal-pane/TerminalPane.tsxsrc/renderer/src/components/terminal-pane/pty-connection-types.tssrc/renderer/src/components/terminal-pane/pty-connection.test.tssrc/renderer/src/components/terminal-pane/pty-connection.tssrc/renderer/src/components/terminal-pane/pty-transport.test.tssrc/renderer/src/components/terminal-pane/pty-transport.tssrc/renderer/src/components/terminal-pane/remote-runtime-pty-transport.test.tssrc/renderer/src/components/terminal-pane/remote-runtime-pty-transport.tssrc/renderer/src/components/terminal-pane/terminal-layout-leaf-ids.test.tssrc/renderer/src/components/terminal-pane/terminal-layout-leaf-ids.tssrc/renderer/src/components/terminal-pane/terminal-shutdown-layout-capture.test.tssrc/renderer/src/components/terminal-pane/terminal-shutdown-layout-capture.tssrc/renderer/src/components/terminal-pane/use-terminal-pane-lifecycle.tssrc/renderer/src/hooks/useIpcEvents.tssrc/renderer/src/store/slices/terminals-hydration.test.tssrc/renderer/src/store/slices/terminals.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/renderer/src/components/terminal-pane/pty-connection.ts (1)
1321-1328: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winClear the stale layout binding for obsolete transport exits.
This branch removes tab ownership but leaves the pane’s persisted PTY binding untouched. If the replacement transport has not spawned/synced yet, the exited
ptyIdcan remain interminalLayoutsByTabIdand be reused by shutdown/hydration. Use the same guarded helper as the normal exit and reattach-expiration paths.🐛 Proposed fix
if (currentPaneTransport && currentPaneTransport !== transport) { // Why: an old transport can deliver a late exit after this pane has // rebound to a replacement PTY; only clear ownership for the exited id. + deps.clearExitedPanePtyLayoutBinding(pane.id, ptyId) deps.clearTabPtyId(deps.tabId, ptyId) deps.consumeSuppressedPtyExit(ptyId) scheduleRuntimeGraphSync() return }src/renderer/src/components/terminal-pane/TerminalPane.tsx (2)
1167-1173: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMove active-pane restoration ahead of the insertion fast-path and before persisting.
This block still sits after the
insertions.length === 0early return, so a host snapshot that only changesactiveLeafIdnever updates the manager’s active pane. And when insertions do happen,persistLayoutSnapshot()runs first, so the store can immediately reserialize the old active leaf.Suggested fix
- if (insertions.length === 0) { - return - } - let appliedInsertion = false for (const insertion of insertions) { // ... } - - if (appliedInsertion) { - persistLayoutSnapshot() - } const activePaneId = restoredLayout.activeLeafId ? manager.getNumericIdForLeaf(restoredLayout.activeLeafId) : null const fallbackActivePaneId = manager.getActivePane()?.id ?? manager.getPanes()[0]?.id ?? null const nextActivePaneId = activePaneId ?? fallbackActivePaneId if (nextActivePaneId !== null) { manager.setActivePane(nextActivePaneId, { focus: isActive }) } + if (appliedInsertion) { + persistLayoutSnapshot() + }
1317-1336: 📐 Maintainability & Code Quality | 🟡 MinorRemove duplicate
clearExitedPanePtyLayoutBindingfrom dependency array
clearExitedPanePtyLayoutBindingappears twice in the dependency array withinsrc/renderer/src/components/terminal-pane/TerminalPane.tsx. This redundancy adds no value and can trigger exhaustive-deps warnings or linter noise.Remove the duplicate entry to maintain clean, idiomatic code.
View snippet
[ clearCodexRestartNotice, clearExitedPanePtyLayoutBinding, clearRuntimePaneTitle, clearTabPtyId, cwd, dispatchNotification, markWorktreeUnread, markTerminalTabUnread, markTerminalPaneUnread, clearWorktreeUnread, clearTerminalTabUnread, clearTerminalPaneUnread, showRestoredSessionBanner, onPtyExitRef, setCacheTimerStartedAt, setRuntimePaneTitle, suppressPtyExit, clearExitedPanePtyLayoutBinding, syncPanePtyLayoutBinding, ]
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 96578cb5-7919-4140-9eb2-cdf73307b98d
📒 Files selected for processing (10)
src/main/runtime/orca-runtime.test.tssrc/main/runtime/orca-runtime.tssrc/renderer/src/components/terminal-pane/TerminalPane.tsxsrc/renderer/src/components/terminal-pane/pty-connection.test.tssrc/renderer/src/components/terminal-pane/pty-connection.tssrc/renderer/src/components/terminal-pane/terminal-layout-leaf-ids.test.tssrc/renderer/src/components/terminal-pane/terminal-layout-leaf-ids.tssrc/renderer/src/components/terminal-pane/terminal-shutdown-layout-capture.test.tssrc/renderer/src/components/terminal-pane/terminal-shutdown-layout-capture.tssrc/renderer/src/hooks/useIpcEvents.ts
✅ Files skipped from review due to trivial changes (2)
- src/renderer/src/components/terminal-pane/terminal-shutdown-layout-capture.ts
- src/renderer/src/hooks/useIpcEvents.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/runtime/orca-runtime.ts
- src/main/runtime/orca-runtime.test.ts
- src/renderer/src/components/terminal-pane/pty-connection.test.ts
* Unify terminal-driven worktree activation and Cmd+J recency marking. * Record worktree visits in the back/forward history stack unless navigating history.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0c71a563-f076-430b-9b39-b23cfa315c6b
📒 Files selected for processing (6)
src/renderer/src/components/terminal-pane/TerminalPane.tsxsrc/renderer/src/components/terminal-pane/terminal-layout-leaf-ids.tssrc/renderer/src/components/terminal-pane/terminal-visibility-resume.tssrc/renderer/src/components/terminal-pane/use-terminal-pane-global-effects.test.tssrc/renderer/src/components/terminal-pane/use-terminal-pane-global-effects.tssrc/renderer/src/components/terminal-pane/use-terminal-window-wake-recovery.ts
💤 Files with no reviewable changes (1)
- src/renderer/src/components/terminal-pane/TerminalPane.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/renderer/src/components/terminal-pane/terminal-layout-leaf-ids.ts
- Repair the active terminal pane leaf selection when a pane's PTY exits, preventing focus from being stranded on a dead pane. - Ensure stale, pruned, or unbound terminal leaves are not persisted as active in visual layouts or shutdown snapshots. - Ignore late exit notifications and subscription end events from old PTY transports or remote stream handles after a reconnect or rebind.
- Redirect active keyboard focus to a live, PTY-backed sibling pane if the focused pane dies, fails to spawn, or is hydrated without a live connection. - Ignore stale data, replay messages, and subscription rejection errors from older PTY sessions after a transport has reconnected. - Avoid persisting stale PTY bindings and dead pane focus inside terminal shutdown layout snapshots.
When switching between different remote terminal handles, debounced input meant for the previous terminal could get incorrectly flushed and sent to the newly attached terminal. Fix this by clearing the batcher's pending state (text and byte counts) and invoking `inputBatcher.clear()` if the attached terminal handle changes.
- Extract and enhance window focus and visibility change recovery logic into a dedicated `useTerminalWindowWakeRecovery` hook. - Recover the xterm renderer, input surface, and WebGL texture atlases when macOS/display wakes or the window/tab regains focus. - Schedule recovery steps with requestAnimationFrame to ensure the terminal layout settles correctly.
AmethystLiang
force-pushed
the
wire-terminal-worktree-navitgation-to-back-and-forth-stack
branch
from
1185580 to
7e3ba42
Compare
Avoid mutating a local `let` variable from inside a Promise executor closure by wrapping the rejection callback in a `const` object.
When window focus and visibility events fire concurrently, they can trigger multiple redundant wake recovery passes. - Return early if a wake recovery frame is already scheduled. - Keep the scheduled animation frame instead of canceling it to ensure we get a settled recovery pass.
…minal-worktree-navitgation-to-back-and-forth-stack
1 participant
Summary
Describe the user-visible change.
Screenshots
No visual change.Testing
pnpm lintpnpm typecheckpnpm testpnpm buildAI Review Report
Summarize the code review you ran with your AI coding agent. Include the main risks it checked, what it flagged, and what you changed or verified as a result.
Confirm that the review explicitly checked cross-platform compatibility for macOS, Linux, and Windows, including shortcuts, labels, paths, shell behavior, and any Electron-specific platform differences touched by this PR.
Security Audit
Provide a basic security audit summary from your AI coding agent. Call out any input handling, command execution, path handling, auth, secrets, dependency, or IPC risks that were reviewed, plus any follow-up needed.
Notes
Call out any platform-specific behavior, risks, or follow-up work.